-
-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simple plugin system #1389
Simple plugin system #1389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the design overall. Added some thoughts.
Yeah, that might make sense. I don't know. After all,
I added a comment for that. Maybe there's a way, but if it makes things too complicated, we can leave it out for now.
For now, we just need to make sure that Extism works with the design we have in mind. We can create a separate branch+PR, which is based on this branch and adds one plugin based on Extism as a proof-of-concept. |
@HU90m, since this also affects our Architecture discussion, it would be great if you could also take a look. 😃 For some background, @thomas-zahner and me talked about a simple plugin-system offline and that it would be based on Extism. This first step is to work on the trait system, which will allow plugins in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this
What's the intended use of the ChainResult::Done
? Is it so just that filters could be part of the request chain or will plugins be able to access IO?
@thomas-zahner can probably give a better answer here, but the idea is that at any point of the chain, a step can decide to stop the execution and return a final conclusion without passing the request on to the next step. Every plugin in the chain would be able to make network requests or access files, so yes, they will be able to access I/O, but I'm not sure what is the relation to |
@HU90m Yes filters are a good example of a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mre The PR is ready for re-review. Basically this is a simple version of the plugin/chain system we could merge. We could still add a response chain and might make further use of Chainable
for other parts. But I think this is enough as a first version.
On thing I can do before merging is to create a branch for the Extism prototype.
Looks fine.
|
@mre See my latest commit for the I've created an extism-prototype branch for a first simple but functional Extism-plugin system. Feel free to give any feedback. |
Also make `Chainable` and `ChainResult` public to support external plugins/handlers.
I think we want to make the You can review and merge into your branch if it looks fine. After that, let's get this shipped. 😅 Sorry for the long review delay. |
Add documentation to `chain` module
Intro
As discussed with you @mre this is a first fully functional version of a simple plugin system in lychee. The first approach of using a middleware system turned out to be more complex than necessary and it would not be possible to use Extism in that case, because there would be no way to pass a
next
function to Extism. Instead I came up withChain
andChainable
. Conceptually having a request chain and a response chain would be very similar to the middleware approach. Instead of anext
function an element in a chain returns aChainResult
which can prematurely return a result in the chain.The field
request_chain
is added toClientBuilder
andClient
. This chain istraverse
d before making a request incheck_website
.check_file
andcheck_mail
are not affected. Remapping, exclusions and caching is not handled by the chain, because "they have no request".Questions and concerns
Field in
Client
andClientBuilder
Currently I have made
request_chain
of typeArc<Mutex<RequestChain>>
. This is becausereqwest::Request
is notClone
butClient
andClientBuilder
areClone
. Do you think it should be done this way? The alternative would probably be to updateRequestChain
to beChain<OurOwnRequestType, Status>
. WhenOurOwnRequestType
implementsCopy
we probably would also facilitate the Extism plugins since copying data from/to the plugins is necessary.Response chain
There is no response chain yet. Should I try to implement one in this PR?
Extism
This PR contains nothing related to Extism yet. I think it would be awesome if WASM plugins could be provided on the command line. So this would probably be lychee-bin loading the plugins and passing it to lychee-lib. Should I try add this in this PR? (edit: see #1418)